Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MD] Address UX comments on index pattern stack #2611

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

kristenTian
Copy link
Contributor

  • Update data source column header in index pattern table
  • Update index pattern column name to Title
  • Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian tyarong@amazon.com

Description

image

image

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@kristenTian kristenTian added the multiple datasource multiple datasource project label Oct 19, 2022
@kristenTian kristenTian requested a review from a team as a code owner October 19, 2022 01:25
@kavilla
Copy link
Member

kavilla commented Oct 19, 2022

Looks like a snapshot failure?

@KrooshalUX
Copy link

LGTM with one slight change - the table column header should not appear sentence case and should read Data Source Connection

@zhongnansu zhongnansu added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 20, 2022
Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #2611 (430962e) into main (195cc8e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #2611   +/-   ##
=======================================
  Coverage   66.78%   66.79%           
=======================================
  Files        3207     3207           
  Lines       61174    61174           
  Branches     9329     9329           
=======================================
+ Hits        40856    40860    +4     
+ Misses      18083    18080    -3     
+ Partials     2235     2234    -1     
Impacted Files Coverage Δ
...ents/step_data_source/components/header/header.tsx 61.11% <0.00%> (ø)
...ic/application/models/sense_editor/sense_editor.ts 64.88% <0.00%> (ø)
packages/osd-optimizer/src/node/cache.ts 52.77% <0.00%> (+2.77%) ⬆️
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 87.75% <0.00%> (+4.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zhongnansu
Copy link
Member

@kristenTian can you change the title to something meaningful? So when we squash and merge. The commit message makes sense for user.

zhongnansu
zhongnansu previously approved these changes Oct 20, 2022
@@ -183,7 +183,7 @@ export const IndexPatternTable = ({ canSave, history }: Props) => {
const columns = [
{
field: 'title',
name: 'Pattern',
name: 'Title',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have insight or if this favorable or not?

Copy link
Contributor Author

@kristenTian kristenTian Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KrooshalUX requested this change. She mentioned to be consistent with save object management table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kristenTian @KrooshalUX - We discussed in the brainstorming meeting today - the "title" reference in the saved object table is a bit misleading, and "pattern" or "index pattern" is the more accurate term. One potential solution in the future would be to disambiguate the index pattern saved object title from the pattern itself: #2649

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just blocking for more insight on the Pattern to Title change

@kristenTian
Copy link
Contributor Author

kristenTian commented Oct 20, 2022

@kristenTian can you change the title to something meaningful? So when we squash and merge. The commit message makes sense for user.

Done I have included all changes the commit message already. So even with squash, it will be visible to user.

@kristenTian kristenTian changed the title [MD] Address UX comments [MD] Address UX comments on index pattern stack Oct 20, 2022
@KrooshalUX
Copy link

KrooshalUX commented Oct 21, 2022

@kavilla As we were working through all of the interfaces for MDS and working to aligning to established patterns, we determined that Index Pattern tables were inconsistent with the pattern across Dashboards, which is to use "Title", as it also reflects the object API.

@kristenTian kristenTian requested a review from kavilla October 21, 2022 20:24
@AMoo-Miki
Copy link
Collaborator

AMoo-Miki commented Oct 21, 2022

I would argue that this interface does not show a "title" or even a "pattern"; it is specifically an "index pattern". While creating an index pattern, OSD doesn't ask for a "title" or "pattern"; it asks the user for an "index pattern".

PS, the index pattern creation page incorrectly calls it "Index pattern name" for a field that is really the "index pattern" and is not a name. We will have to fix that separately.

edit: wrote this in a hurry and reading it out loud, i didn't like the tune; rewording it.

@kristenTian
Copy link
Contributor Author

I would argue that this interface does not show a "title" or even a "pattern"; it is specifically an "index pattern". While creating an index pattern, you don't get to set a "name" or "title"; you only get to set an "index pattern".

PS, the index pattern creation page incorrectly calls it "Index pattern name" for a field that is really the "index pattern" and is not a name. We will have to fix that separately.

This make sense to me, however still requires a further alignment on UX. Given the PR contains other small issues to address, I will revert the column title change for now and address it in a separate PR.

@KrooshalUX
Copy link

KrooshalUX commented Oct 21, 2022

We can move forward without the change to Pattern / Title - given the discussion around naming, APIs and table patterns that needs to happen at a larger scale.

@kristenTian kristenTian force-pushed the OSD-main branch 2 times, most recently from 1a8268d to bc85910 Compare October 21, 2022 23:00
    - Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>
Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kristenTian kristenTian merged commit 189cf81 into opensearch-project:main Oct 22, 2022
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2611-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 189cf81a185f59a934d4d593206f7a115ebbd399
# Push it to GitHub
git push --set-upstream origin backport/backport-2611-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2611-to-2.x.

zhongnansu pushed a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Oct 24, 2022
…2611)

- Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
(cherry picked from commit 189cf81)
zhongnansu pushed a commit to zhongnansu/OpenSearch-Dashboards that referenced this pull request Oct 24, 2022
…2611)

- Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
(cherry picked from commit 189cf81)
Signed-off-by: Su <szhongna@amazon.com>
zhongnansu added a commit that referenced this pull request Oct 25, 2022
- Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
(cherry picked from commit 189cf81)
Signed-off-by: Su <szhongna@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
Signed-off-by: Su <szhongna@amazon.com>
Co-authored-by: Kristen Tian <105667444+kristenTian@users.noreply.github.com>
@AMoo-Miki AMoo-Miki added enhancement New feature or request ux / ui Improvements or additions to user experience, flows, components, UI elements labels Nov 5, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
…2611)

- Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
Signed-off-by: Sergey V. Osipov <sipopo@yandex.ru>
Arpit-Bandejiya pushed a commit to Arpit-Bandejiya/OpenSearch-Dashboards that referenced this pull request Jan 13, 2023
…2611)

- Update data source column header in index pattern table
    - Update index pattern column name to Title
    - Remove data source search field error check given it is not required

Signed-off-by: Kristen Tian <tyarong@amazon.com>

Signed-off-by: Kristen Tian <tyarong@amazon.com>
Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request multiple datasource multiple datasource project ux / ui Improvements or additions to user experience, flows, components, UI elements v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants